Fix CPU / thread locks + improve performance of BrowserBookmark plugin #3815
Conversation
## 1. High CPU and UI Freezing * **Initial Load Spike:** The original plugin caused a massive CPU spike on first load due to inefficient database queries for favicons. **Fix:** We now create temporary indexes on the database copies, making the initial favicon scan virtually instantaneous. * **Query-Time CPU Usage:** Typing in the search bar caused high CPU because the plugin checked for the existence of every icon file on every keystroke. **Fix:** We now pre-validate all icon paths once during the initial load, making search queries extremely fast, in-memory operations. * **Background CPU Churn:** Actively browsing the web would trigger constant, high-CPU reloads of the plugin's data. **Fix:** We implemented a debouncing mechanism for the file watcher, ensuring that even a storm of file changes only results in a single, efficient data reload. * **UI Freezing on Scroll (The Final Bug):** The most severe issue was the UI locking up with 100% CPU on multiple threads when scrolling through results. This was caused by the UI's renderer (SharpVectors) attempting to render corrupt SVG favicons created by a race condition. **Fix:** We now convert **all** favicons to PNG format during the loading process. This guarantees that the UI never receives a corrupt or incompatible image, permanently solving the freezing issue. ## 2. Improved Reliability and Functionality * **Fixed Firefox Profile Discovery:** The plugin now reliably and automatically discovers the default Firefox profile, even in non-standard locations or with different naming schemes, by correctly parsing the `profiles.ini` file. * **Fixed Real-Time Settings Changes:** Toggling the "Load favicons" checkbox now triggers an immediate data reload, so the change takes effect instantly without requiring an application restart. ## 3. Enhanced Code Quality and User Experience * **Asynchronous Startup:** The entire data loading process is now fully asynchronous. The plugin displays an "initializing..." message while loading in the background, ensuring the Flow Launcher UI remains responsive at all times. * **Code Stability:** eliminated unsafe `async void` methods, fixed various compilation errors, and centralized duplicated code into a shared helper class, making the plugin more stable and maintainable.
|
🥷 Code experts: Jack251970, onesounds Jack251970, onesounds have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 Walkthrough## Walkthrough
This update refactors and improves the bookmark and favicon loading logic for Chromium and Firefox browsers in the BrowserBookmark plugin. It introduces robust error handling, centralized favicon processing with image resizing and SVG support, improved concurrency and initialization control, and enhanced file monitoring with debounced reloads. Settings and UI bindings are also streamlined.
## Changes
| File(s) | Change Summary |
|---------------------------------------------------------------------------------------------|---------------|
| .../ChromiumBookmarkLoader.cs | Added error handling for JSON parsing; refined bookmark type checks; refactored favicon loading to use a new helper; removed unused usings. |
| .../FirefoxBookmarkLoader.cs | Rewrote profile path parsing; replaced LINQ with explicit loops; consolidated favicon loading via helper; removed extension method and unused usings. |
| .../Flow.Launcher.Plugin.BrowserBookmark.csproj | Upgraded Microsoft.Data.Sqlite package; added SixLabors.ImageSharp, SkiaSharp, and Svg.Skia package references. |
| .../Helper/FaviconHelper.cs | Renamed and refactored temp DB handling method; added SVG detection and conversion to PNG; enhanced image resizing and error handling; removed old SVG detection method. |
| .../Main.cs | Refactored initialization and reload logic with async, thread-safe handling; improved file watching with debounce; updated query and dispose logic. |
| .../Views/CustomBrowserSetting.xaml | Removed a guide message TextBlock from the UI. |
| .../Views/SettingsControl.xaml | Changed CheckBox binding for "Enable Favicons" to bind directly to `EnableFavicons`. |
| .../Views/SettingsControl.xaml.cs | Added `EnableFavicons` property; simplified async calls; updated event handling for settings changes; removed unnecessary imports. |
| .../CustomChromiumBookmarkLoader.cs | Added missing `using System.IO;` directive. |
| .../Languages/*.xaml | Added new localization string key `flowlauncher_plugin_browserbookmark_plugin_initializing` with translations in multiple languages. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Main
participant BookmarkLoader
participant FaviconHelper
participant FileSystem
User->>Main: Start plugin / change settings
Main->>Main: Acquire initialization semaphore
Main->>BookmarkLoader: Load bookmarks (async)
BookmarkLoader->>FaviconHelper: ProcessFavicons(bookmarks, db, ...)
FaviconHelper->>FaviconHelper: Extract, resize, save favicons
FaviconHelper-->>BookmarkLoader: Update bookmark favicon paths
BookmarkLoader-->>Main: Return bookmarks
Main->>Main: Set initialized flag
FileSystem-->>Main: Bookmark file changed event
Main->>Main: Debounce reload (3s)
Main->>BookmarkLoader: Reload bookmarks and faviconsPossibly related issues
Possibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (1)
27-29: Consider logging index creation failures for better diagnostics.While ignoring index creation failures when indexes already exist is reasonable, completely swallowing
SqliteExceptionwithout any logging could hide other issues like database corruption or permission problems.-try { command.ExecuteNonQuery(); } catch (SqliteException) { /* ignore */ } +try { command.ExecuteNonQuery(); } catch (SqliteException ex) { Main._context.API.LogInfo(ClassName, $"Index creation skipped (likely already exists): {ex.Message}"); }Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
89-89: Consider extracting the hardcoded fallback message to a constant.The hardcoded English message could be defined as a constant for better maintainability.
+private const string FallbackInitializingMessage = "Plugin is initializing, please try again in a few seconds"; + public List<Result> Query(Query query) { // ... existing code ... - var initializingSubTitle = "Plugin is initializing, please try again in a few seconds"; + var initializingSubTitle = FallbackInitializingMessage;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs(3 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(5 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml(0 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs(7 hunks)
💤 Files with no reviewable changes (1)
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml
🧰 Additional context used
🧠 Learnings (7)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj (5)
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2442
File: Flow.Launcher.Plugin/Flow.Launcher.Plugin.csproj:4-4
Timestamp: 2024-08-31T20:42:08.482Z
Learning: In .NET 8, SourceLink is included by default, eliminating the need for the `Microsoft.SourceLink.GitHub` package.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2442
File: Flow.Launcher.Plugin/Flow.Launcher.Plugin.csproj:4-4
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In .NET 8, SourceLink is included by default, eliminating the need for the `Microsoft.SourceLink.GitHub` package.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml (3)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#0
File: :0-0
Timestamp: 2025-04-23T15:14:49.986Z
Learning: In WPF applications like Flow.Launcher, font styling should be applied using implicit styles instead of setting the FontFamily property on individual controls. Define implicit styles in a ResourceDictionary using <Style TargetType="{x:Type Button}"> format and merge it into App.xaml, which automatically applies the font to all instances of the control type while still allowing explicit overrides where needed.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (4)
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Learnt from: onesounds
PR: Flow-Launcher/Flow.Launcher#0
File: :0-0
Timestamp: 2025-04-23T15:14:49.986Z
Learning: In WPF applications like Flow.Launcher, font styling should be applied using implicit styles instead of setting the FontFamily property on individual controls. Define implicit styles in a ResourceDictionary using <Style TargetType="{x:Type Button}"> format and merge it into App.xaml, which automatically applies the font to all instances of the control type while still allowing explicit overrides where needed.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs (5)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#3081
File: Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs:136-145
Timestamp: 2025-02-22T23:51:54.010Z
Learning: In Flow.Launcher JsonRPCV2 plugins, the `reload_data` method is optional for plugins to implement. When the method is not found (RemoteMethodNotFoundException), it should be silently caught without logging as this is an expected scenario.
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (2)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (3)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3279
File: Flow.Launcher/Helper/WallpaperPathRetrieval.cs:44-46
Timestamp: 2025-02-28T07:47:24.148Z
Learning: In Flow.Launcher's WallpaperPathRetrieval class, using a `using` statement with MemoryStream when loading images with BitmapImage does not work properly, even when using BitmapCacheOption.OnLoad. The stream needs to remain open while the bitmap is in use.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (2)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs
[warning] 151-151:
Favicons is not a recognized word. (unrecognized-spelling)
[warning] 140-140:
Favicons is not a recognized word. (unrecognized-spelling)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (15)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml (1)
101-101: LGTM! Binding simplified correctly.The binding change aligns with the new
EnableFaviconsproperty in the code-behind file, which properly encapsulates the setting update logic and triggers bookmark reloads.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj (1)
100-100: No changes needed: SixLabors.ImageSharp 3.1.10 is up-to-date and secureVersion 3.1.10 (released June 13, 2025) is the latest stable release and already includes fixes for all known advisories:
- CVE-2024-32035 and CVE-2024-32036 (fixed in 3.1.4)
- CVE-2024-27929 (fixed in 3.1.3)
File to verify remains unchanged:
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj, line 100
No action required.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs (4)
1-10: Good cleanup of imports and class declaration.Removing the unused
System.Threading.Tasksimport and explicitly inheriting fromUserControlimproves code clarity.
27-27: Consistent async pattern for bookmark reloads.The change from
Task.Run(() => Main.ReloadAllBookmarks())to_ = Main.ReloadAllBookmarks()is appropriate sinceReloadAllBookmarks()is already async. This eliminates unnecessary thread pool usage while maintaining fire-and-forget semantics.Also applies to: 37-37, 47-48, 79-79, 88-88, 120-120
51-60: Well-implemented EnableFavicons property.The property follows the established pattern, properly updating the setting, triggering bookmark reloads, and raising property change notifications for UI binding.
76-76: Cleaner dialog result check.The simplified check
if (window.ShowDialog() == true)is more idiomatic and readable.Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs (3)
79-90: Excellent error handling for JSON parsing.Adding try-catch for
JsonExceptionprevents crashes from corrupt bookmark files and provides helpful error logging for debugging.
124-130: Robust bookmark enumeration with proper validation.The explicit checks for both
nameandurlproperties before creating a bookmark prevent potential null reference issues and ensure data integrity.
140-160: Well-refactored favicon loading using centralized helper.The delegation to
FaviconHelper.ProcessFaviconsimproves code reusability and maintainability. The SQL query is properly formatted and the lambda expressions for data extraction are clear and concise.Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (3)
66-77: Clear and straightforward bookmark reading logic.Replacing the custom LINQ extension with an explicit
whileloop improves readability and removes unnecessary complexity. The null handling for the title field is appropriate.
120-139: Consistent favicon loading implementation.The refactoring to use
FaviconHelper.ProcessFaviconsmaintains consistency withChromiumBookmarkLoaderand properly handles the Firefox database schema. The comment about PNG conversion is helpful.
206-273: Excellent INI parsing implementation with robust profile resolution.The rewritten
GetProfileIniPathmethod is significantly more robust:
- Proper INI file parsing into a structured dictionary
- Two-strategy approach for finding the default profile
- Correct handling of relative vs absolute paths
- Comprehensive error handling
This addresses the Firefox profile discovery issues mentioned in the PR objectives.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (1)
102-128: Excellent refactoring to handle multiple image formats safely!The use of ImageSharp to load, resize, and convert images to PNG is a robust solution that prevents malformed images (especially SVGs) from crashing the UI. The error handling ensures that processing continues even if individual favicons fail.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (2)
169-190: Well-implemented debouncing mechanism!The use of
CancellationTokenSourcewithInterlocked.Exchangeeffectively prevents rapid file change events from triggering multiple reloads. The 3-second delay is reasonable for filesystem operations.
194-194: Fire-and-forget pattern is acceptable here due to existing error handling.While the discard operator usage could hide exceptions,
ReloadAllBookmarksalready contains comprehensive error handling that logs any failures, making this pattern acceptable in this context.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs
Outdated
Show resolved
Hide resolved
|
Note - still an issue with some SVG favicons not being picked up. I'll address that if / when this PR is accepted. |
Thank you for the contribution! I will review it in my spare time. |
There was a problem hiding this comment.
Pull Request Overview
Performance and stability improvements for the BrowserBookmark plugin, including asynchronous startup, debounced file watching, and guaranteed PNG favicons.
- Load and refresh bookmarks asynchronously with a semaphore to prevent races and UI blocking
- Pre-validate and convert all favicons to PNG using a new shared helper and ImageSharp
- Refactor settings UI and browser loaders for real-time toggles, improved profile parsing, and removal of unsafe async void methods
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Views/SettingsControl.xaml.cs | Extended from UserControl, removed Task.Run wrappers, added EnableFavicons property |
| Views/SettingsControl.xaml | Updated IsChecked binding to use the new EnableFavicons property |
| Views/CustomBrowserSetting.xaml | Removed an outdated guide message TextBlock |
| Main.cs | Added async initialization, semaphore, debounced file watcher, and made ReloadAllBookmarks async |
| Helper/FaviconHelper.cs | Introduced ProcessFavicons to index and convert all favicons to PNG |
| Flow.Launcher.Plugin.BrowserBookmark.csproj | Added SixLabors.ImageSharp package |
| FirefoxBookmarkLoader.cs | Refactored to use ProcessFavicons, improved profiles.ini parsing |
| ChromiumBookmarkLoader.cs | Refactored favicon loading to use ProcessFavicons, added JSON parse error handling |
Comments suppressed due to low confidence (5)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:200
- [nitpick]
GetProfileIniPathis quite long and handles multiple parsing strategies. To improve readability, consider extracting ini‐section parsing and default‐profile selection into smaller helper methods with comments.
private static string GetProfileIniPath(string profileFolderPath)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/CustomBrowserSetting.xaml:101
- [nitpick] You removed the guide message TextBlock bound to
flowlauncher_plugin_browserbookmark_guideMessage03. Ensure the corresponding translation key or resource is cleaned up to avoid orphaned entries.
FontSize="14"
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs:135
- [nitpick] Logging the entire JSON fragment can be hard to read. Consider logging just the element's type or name (e.g.,
subElement.GetProperty("type").GetString()) to make this error more actionable.
Main._context.API.LogError(ClassName, $"type property not found for {subElement.ToString()}");
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs:46
ProcessFaviconsis a key public API for the plugin’s favicon logic. Adding XML doc comments explaining parameters (e.g.,sqlQuery,patternPrefix) and behavior would help future maintainers.
public static void ProcessFavicons(
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs:51
- [nitpick] The new asynchronous startup (
LoadBookmarksInBackgroundAsync) and file‐watch debouncing are critical flows but aren’t covered by tests. Consider adding unit or integration tests to verify initialization and debouncing behavior.
private async Task LoadBookmarksInBackgroundAsync()
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
48-48: The fire-and-forget pattern still lacks exception handling.This is the same issue identified in the previous review. The background initialization task could fail silently, leaving the plugin uninitialized indefinitely.
Apply this fix to ensure initialization failures are logged:
- _ = LoadBookmarksInBackgroundAsync(); + _ = Task.Run(async () => + { + try + { + await LoadBookmarksInBackgroundAsync(); + } + catch (Exception ex) + { + _context.API.LogException(ClassName, "Failed to initialize bookmarks", ex); + } + });
🧹 Nitpick comments (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (2)
113-136: Optimize performance by avoiding unnecessary null filtering.The current implementation creates null entries that are filtered out later. Consider using
Whereclause beforeSelectto avoid unnecessary object creation.Apply this diff to improve performance:
- var results = _cachedBookmarks - .Select(c => - { - var score = topResults ? 5 : BookmarkLoader.MatchProgram(c, param).Score; - if (!topResults && score <= 0) - return null; - - return new Result - { - Title = c.Name, - SubTitle = c.Url, - IcoPath = c.FaviconPath, // Use the pre-validated path directly. - Score = score, - Action = _ => - { - _context.API.OpenUrl(c.Url); - return true; - }, - ContextData = new BookmarkAttributes { Url = c.Url } - }; - }) - .Where(r => r != null); + var results = _cachedBookmarks + .Select(c => new { Bookmark = c, Score = topResults ? 5 : BookmarkLoader.MatchProgram(c, param).Score }) + .Where(x => topResults || x.Score > 0) + .Select(x => new Result + { + Title = x.Bookmark.Name, + SubTitle = x.Bookmark.Url, + IcoPath = x.Bookmark.FaviconPath, + Score = x.Score, + Action = _ => + { + _context.API.OpenUrl(x.Bookmark.Url); + return true; + }, + ContextData = new BookmarkAttributes { Url = x.Bookmark.Url } + });
149-149: Improve filter comparison for better performance.The string comparison with
OrdinalIgnoreCaseis unnecessarily complex for filename comparison where exact matching is typically sufficient.Consider simplifying the comparison:
- if (Watchers.Any(x => x.Path.Equals(directory, StringComparison.OrdinalIgnoreCase) && x.Filter == Path.GetFileName(path))) + if (Watchers.Any(x => x.Path == directory && x.Filter == Path.GetFileName(path)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(4 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs
🧰 Additional context used
🧠 Learnings (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (3)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3118
File: Flow.Launcher/ViewModel/MainViewModel.cs:1404-1413
Timestamp: 2024-12-08T21:12:12.060Z
Learning: In the `MainViewModel` class, the `_lastQuery` field is initialized in the constructor and is never null.
🔇 Additional comments (6)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (6)
27-35: LGTM: Well-designed concurrency control fields.The addition of instance tracking, thread-safe initialization flag, semaphore for concurrent loading prevention, and debounce token source demonstrates good architectural improvements for addressing the performance issues mentioned in the PR.
51-81: Excellent async initialization with proper concurrency control.The implementation correctly uses a semaphore to prevent concurrent loading operations, validates the cache directory, and pre-validates favicon paths to improve query performance. The thread-safe design with proper resource management is well done.
86-108: Good user experience improvement during initialization.Providing immediate feedback to users when the plugin is still initializing is a significant improvement over silently returning no results. The fallback handling for missing translation keys is also well implemented.
169-190: Well-implemented debouncing mechanism with proper cancellation.The debouncing logic correctly cancels previous delay tasks when new file changes occur, preventing excessive reload operations. The exception handling for
TaskCanceledExceptionis appropriate for the debouncing pattern.
197-214: Robust async reload implementation with proper error handling.The method correctly resets the initialization state, clears cached data, and provides comprehensive exception logging. The conditional disposal of file watchers is a good design choice for different reload scenarios.
268-270: Proper cleanup of debounce resources in Dispose.The cancellation and disposal of the debounce token source ensures proper resource cleanup when the plugin is disposed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
48-48: Fix unhandled exceptions in fire-and-forget pattern.The current implementation uses a fire-and-forget pattern that could hide initialization failures. If
LoadBookmarksInBackgroundAsyncthrows an unhandled exception, it could crash the application or leave the plugin uninitialized indefinitely.Apply the suggested fix from the previous review to wrap the call in proper exception handling.
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
71-77: Consider async I/O for file existence checks.The pre-validation of favicon paths uses synchronous
File.Exists()calls in a loop, which could block the thread for a significant time if there are many bookmarks or if the file system is slow.// Pre-validate all icon paths once to avoid doing it on every query -foreach (var bookmark in _cachedBookmarks) -{ - if (string.IsNullOrEmpty(bookmark.FaviconPath) || !File.Exists(bookmark.FaviconPath)) - { - bookmark.FaviconPath = DefaultIconPath; - } -} +await Task.Run(() => +{ + Parallel.ForEach(_cachedBookmarks, bookmark => + { + if (string.IsNullOrEmpty(bookmark.FaviconPath) || !File.Exists(bookmark.FaviconPath)) + { + bookmark.FaviconPath = DefaultIconPath; + } + }); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (5)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:244-247
Timestamp: 2025-06-08T14:12:21.348Z
Learning: In Flow.Launcher, App.NotifyIcon is created before MainWindow creation, so null checks for App.NotifyIcon are not necessary when accessing it from MainWindow code.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3672
File: Flow.Launcher/MainWindow.xaml.cs:318-318
Timestamp: 2025-06-08T14:12:12.842Z
Learning: In Flow.Launcher, the App.NotifyIcon static property is initialized in the App class before MainWindow creation, so null checks are not needed when accessing App.NotifyIcon in MainWindow lifecycle methods.
Learnt from: Yusyuriv
PR: Flow-Launcher/Flow.Launcher#3057
File: Flow.Launcher.Core/Plugin/JsonRPCPluginSettings.cs:0-0
Timestamp: 2024-11-03T07:40:11.014Z
Learning: In Flow Launcher, when using Windows Forms dialogs (e.g., in `JsonRPCPluginSettings.cs`), path validation is enabled by default in `OpenFileDialog` and `FolderBrowserDialog`, preventing users from selecting invalid paths, but it's possible to opt out of this validation on individual dialogs.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Report (PR)
🔇 Additional comments (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (3)
15-15: Excellent async refactoring implementation.The transition from
IPlugintoIAsyncPluginis well-implemented with proper use of semaphores for thread-safe initialization. The background loading approach with the "initializing..." message (mentioned in PR objectives) should significantly improve user experience by keeping the UI responsive.Also applies to: 37-50, 52-85
169-190: Potential resource leak in debounce cancellation handling.The debouncing implementation correctly handles cancellation but has a subtle issue: if multiple rapid file changes occur, the old
CancellationTokenSourcemight not be properly disposed before being replaced, leading to potential resource leaks over time.private static void OnBookmarkFileChanged(object sender, FileSystemEventArgs e) { - var oldCts = Interlocked.Exchange(ref _debounceTokenSource, new CancellationTokenSource()); - oldCts?.Cancel(); - oldCts?.Dispose(); + var newCts = new CancellationTokenSource(); + var oldCts = Interlocked.Exchange(ref _debounceTokenSource, newCts); + try + { + oldCts?.Cancel(); + } + finally + { + oldCts?.Dispose(); + } - var newCts = _debounceTokenSource;Likely an incorrect or invalid review comment.
87-106: Fix potential deadlock in QueryAsync method.There's a logical flaw in the async query implementation. When
_isInitializedisfalse, the method correctly waits for the semaphore. However, if initialization is still running inLoadBookmarksInBackgroundAsync, this creates a potential deadlock since both methods try to acquire the same semaphore.public async Task<List<Result>> QueryAsync(Query query, CancellationToken token) { if (!_isInitialized) { - // If the list is not initialized, we need to wait for the list to be refreshed before querying - await _initializationSemaphore.WaitAsync(token); - try - { - return QueryResults(query); - } - finally - { - _initializationSemaphore.Release(); - } + // Wait for initialization to complete without holding the semaphore + while (!_isInitialized) + { + await Task.Delay(50, token); + } + return QueryResults(query); } else { return QueryResults(query); } }Likely an incorrect or invalid review comment.
| @@ -101,13 +101,6 @@ | |||
| FontSize="14" | |||
| Text="{DynamicResource flowlauncher_plugin_browserbookmark_guideMessage02}" | |||
| TextWrapping="WrapWithOverflow" /> | |||
| <TextBlock | |||
There was a problem hiding this comment.
flowlauncher_plugin_browserbookmark_guideMessage03 string does not exist, so we should remove this
|
@dcog989 From my observation, I noticed these two problems:
Original: After refactor:
|
|
Thanks, looking at these now. |
What is the path to your profile, please? |
%AppData%\Roaming\Mozilla\Firefox\profiles.ini |
|
I think my profile is not parsed correctly and that function returns empty. 
|
|
This is the right path: |
|
And this is my profile file content: |
Just need to do some testing and will send updates. |
…989/Flow.Launcher into BrowserBookmark-big-bug-fix
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs (1)
192-193: Remove redundant SqliteConnection.ClearPool call.Since the connection uses
Pooling=false, callingSqliteConnection.ClearPool(connection)is redundant and can be removed.- SqliteConnection.ClearPool(connection);Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
57-57: Consider handling potential exceptions from the background initialization task.The fire-and-forget pattern could hide initialization failures. If
LoadBookmarksInBackgroundAsyncthrows an unhandled exception, the plugin could remain uninitialized indefinitely.- _ = LoadBookmarksInBackgroundAsync(); + _ = Task.Run(async () => + { + try + { + await LoadBookmarksInBackgroundAsync(); + } + catch (Exception ex) + { + _context.API.LogException(ClassName, "Failed to initialize bookmarks", ex); + } + });
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)
238-240: Potential race condition in token exchange.While the
Interlocked.Exchangeprevents most race conditions, there's a small window where the old token source could be disposed while still being used in another thread.Consider using a more robust pattern:
- var oldCts = Interlocked.Exchange(ref _debounceTokenSource, new CancellationTokenSource()); - oldCts?.Cancel(); - oldCts?.Dispose(); + var newCts = new CancellationTokenSource(); + var oldCts = Interlocked.Exchange(ref _debounceTokenSource, newCts); + try + { + oldCts?.Cancel(); + } + finally + { + oldCts?.Dispose(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs(4 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/CustomChromiumBookmarkLoader.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs(4 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/ar.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/cs.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/da.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/de.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/en.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/es-419.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/es.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/fr.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/he.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/it.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/ja.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/ko.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/nb.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/nl.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/pl.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/pt-br.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/pt-pt.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/ru.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/sk.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/sr.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/tr.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/uk-UA.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/vi.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/zh-cn.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/zh-tw.xaml(1 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(4 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs(6 hunks)
✅ Files skipped from review due to trivial changes (26)
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/CustomChromiumBookmarkLoader.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/fr.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/sr.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/en.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/it.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/vi.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/ru.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/es-419.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/da.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/ar.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/pt-br.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/pt-pt.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/es.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/nl.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/ja.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/zh-cn.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/de.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/sk.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/nb.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/cs.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/pl.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/zh-tw.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/ko.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/tr.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/he.xaml
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Languages/uk-UA.xaml
🚧 Files skipped from review as they are similar to previous changes (3)
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/Views/SettingsControl.xaml.cs
- Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (3)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (3)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3279
File: Flow.Launcher/Helper/WallpaperPathRetrieval.cs:44-46
Timestamp: 2025-02-28T07:47:24.148Z
Learning: In Flow.Launcher's WallpaperPathRetrieval class, using a `using` statement with MemoryStream when loading images with BitmapImage does not work properly, even when using BitmapCacheOption.OnLoad. The stream needs to remain open while the bitmap is in use.
Learnt from: NoPlagiarism
PR: Flow-Launcher/Flow.Launcher#3104
File: Flow.Launcher.Plugin/Result.cs:73-74
Timestamp: 2024-12-02T07:37:46.838Z
Learning: In 'Result.cs', the user prefers not to add additional data URI validation in the 'IcoPath' property setter.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs (3)
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3664
File: Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs:326-326
Timestamp: 2025-06-06T05:26:32.163Z
Learning: In the Firefox bookmark loader (FirefoxBookmarkLoader.cs), using hardcoded Windows line endings ("\r\n") for splitting profiles.ini content is acceptable and platform-agnostic line splitting is not required.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
🧬 Code Graph Analysis (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs (4)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (4)
Main(15-352)RegisterBrowserDataDirectory(202-227)List(121-197)List(302-329)Plugins/Flow.Launcher.Plugin.BrowserBookmark/FirefoxBookmarkLoader.cs (4)
LoadFaviconsFromDb(113-181)List(24-24)List(36-111)List(189-195)Plugins/Flow.Launcher.Plugin.BrowserBookmark/CustomChromiumBookmarkLoader.cs (1)
List(18-18)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (3)
FaviconHelper(13-137)ExecuteWithTempDb(17-45)SaveBitmapData(104-136)
🪛 GitHub Check: Check Spelling
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs
[warning] 128-128:
Favicons is not a recognized word. (unrecognized-spelling)
🔇 Additional comments (15)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/ChromiumBookmarkLoader.cs (5)
29-31: Good improvement to file watching strategy.Switching from per-file to directory-level monitoring is more efficient and will catch journal file updates that were previously missed.
67-78: Excellent error handling addition.The JSON parsing is now properly wrapped in try-catch to prevent crashes from malformed bookmark files. The error logging provides good diagnostic information.
112-118: Good refinement of bookmark type handling.Explicitly checking for "url" type and validating required properties prevents invalid bookmarks from being added. This is more robust than the previous default case.
156-160: GLOB pattern usage improves favicon matching.The switch from LIKE to GLOB pattern with host-based matching is more precise and should improve favicon lookup accuracy.
176-182: Excellent duplicate prevention logic.The
ConcurrentDictionaryusage withTryAddefficiently prevents duplicate favicon saves while ensuring all bookmarks get the correct favicon path assigned.Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs (5)
17-45: Well-implemented database optimization strategy.The temporary database copy with indexing is a smart approach to improve query performance without modifying the original database. The exception handling and cleanup logic are robust.
47-52: Simple and effective SVG detection.The SVG detection logic properly handles both plain SVG files and those with XML declarations. Limiting the check to the first 100 bytes is efficient.
54-102: Comprehensive SVG to PNG conversion.The conversion logic properly handles SVG scaling to fit the target canvas and includes appropriate error handling. The 64x64 target size is reasonable for favicon display.
104-136: Enhanced image processing with proper error handling.The combined SVG detection and raster image processing provides a complete solution. The resize-to-max logic ensures consistent favicon sizes while preserving aspect ratios.
58-61: No change needed: disposing MemoryStream after SKSvg.Load is safeSkiaSharp’s
SKSvg.Load(stream)fully reads and parses the SVG into memory, so the input stream can be disposed immediately. You can keep the existing code in
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Helper/FaviconHelper.cs:58-61as-is:using var stream = new MemoryStream(svgData); using var svg = new SKSvg(); svg.Load(stream);Likely an incorrect or invalid review comment.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (5)
63-118: Excellent async initialization with proper concurrency control.The semaphore usage prevents concurrent initialization operations, and the try-finally pattern ensures the plugin always reaches an initialized state even on failure. The favicon cache clearing is a good practice.
125-151: Smart periodic reload strategy for Firefox.The double-checked locking pattern and periodic reload logic effectively addresses the Firefox file watching challenges mentioned in the PR objectives while avoiding excessive reloads.
154-168: Good user feedback during initialization.Providing immediate feedback to users during the initialization process improves the user experience compared to showing no results or errors.
229-261: Well-implemented debounced file watching.The debouncing mechanism with cancellation tokens effectively prevents excessive reloads while ensuring changes are eventually processed. The event filtering is efficient.
339-341: Proper cleanup of cancellation tokens.The cancellation token disposal in the Dispose method prevents resource leaks and ensures proper cleanup.
|
that was a lot more complex than I expected - issues with Firefox favicon updates, etc. I also had issues merging some intermediate commits, so they they may need reviewing / applying again. Sorry for the extra work. However, it all seems to be working and performing well. |
|
I've just spotted a minor issue I want to fix. Please hold the next review. |
|
The original plugin had many issues, including checking So, I've now largely rewritten the entire plugin. I'm therefore closing this PR and will submit a new one once I'm entirely happy with it. It will be a complete refactor. Before that, I will submit a much smaller PR that fixes only the '100% CPU' issue. |
1. High CPU and UI Freezing
2. Improved Reliability and Functionality
profiles.inifile.3. Enhanced Code Quality and User Experience
async voidmethods, fixed various compilation errors, and centralized duplicated code into a shared helper class, making the plugin more stable and maintainable.